PR="299578"; \
SYSTEM="x86_64-linux"; \
CUDA_SUPPORT="true"; \
CUDA_CAPABILITIES='[ "7.5" ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 1" \
  --skip-package-regex 'python3(\d){1,2}Packages\.(pytorch-pfn-extras|ffcv)' \
  --skip-package-regex 'python3(\d){1,2}Packages\.(shap|mlflow|optuna)' \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

Result of nixpkgs-review pr 299578 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.5" ]; }' run on x86_64-linux 1

PR="299578"; \
SYSTEM="aarch64-darwin"; \
CUDA_SUPPORT="false"; \
CUDA_CAPABILITIES='[ ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 1" \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

Result of nixpkgs-review pr 299578 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on aarch64-darwin 1

PR="299578"; \
SYSTEM="x86_64-darwin"; \
CUDA_SUPPORT="false"; \
CUDA_CAPABILITIES='[ ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 1" \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

Result of nixpkgs-review pr 299578 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on x86_64-darwin 1

PR="299578"; \
SYSTEM="aarch64-linux"; \
CUDA_SUPPORT="true"; \
CUDA_CAPABILITIES='[ "7.2" ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 4" \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

Result of nixpkgs-review pr 299578 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.2" ]; }' run on aarch64-linux 1

PR="299578"; \
SYSTEM="aarch64-linux"; \
CUDA_SUPPORT="true"; \
CUDA_CAPABILITIES='[ "7.5" ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 4" \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

Result of nixpkgs-review pr 299578 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.5" ]; }' run on aarch64-linux 1

Thanks for doing this @ConnorBaker ! I'm a fan of all things automated formatting.

How will we enforce this formatting standard in future PRs? Once things are all pretty and pristine after this PR, what's to stop someone from unknowingly breaking formatting in a subsequent PR?

How will we enforce this formatting standard in future PRs? Once things are all pretty and pristine after this PR, what's to stop someone from unknowingly breaking formatting in a subsequent PR?

I don't know :(

As far as I can tell, we don't have any sort of pre-commit hook setup with Nixpkgs. I suppose I could add a new workflow which checks to make sure that the paths formatted by this PR are unchanged when the formatter is run again, but I'm not sure if that's something which is allowed.

Any ideas?

I suppose I could add a new workflow which checks to make sure that the paths formatted by this PR are unchanged when the formatter is run again, but I'm not sure if that's something which is allowed.

A GitHub Actions workflow? Yeah I think that makes sense. IIUC there's a --verify option that we can make use of.

Perhaps the NixOS/rfcs#166 authors (cc @piegamesde @infinisil) might have input on how to enforce the new formatting standard in CI?

@samuela see b6024fa48c4240837f276ca603f8176052c7de75 -- I use the --check argument so the command should fail with an error if a file isn't formatted.

@samuela see b6024fa -- I use the --check argument so the command should fail with an error if a file isn't formatted.

Woohoo! Looks great!

Am I seeing correctly that the reformat is causing a rebuild? If so, would you please investigate this and open a bug report? Nixfmt should never change the code's semantics, and thus never cause any rebuilds.

Oh and with a more generic check, the @NixOS/nix-formatting team should be added as the workflows code owner

I suppose I could add a new workflow which checks to make sure that the paths formatted by this PR are unchanged when the formatter is run again, but I'm not sure if that's something which is allowed.

A GitHub Actions workflow? Yeah I think that makes sense. IIUC there's a --verify option that we can make use of.

Perhaps the NixOS/rfcs#166 authors (cc @piegamesde @infinisil) might have input on how to enforce the new formatting standard in CI?

I would make a GIthub action workflow to just run the changed files in the PR through the formatter and if any diff arises after the run -> report to the PR and block it from merging (basically happens directly because this workflow would be unsuccessful).

if any diff arises after the run -> report to the PR and block it from merging

Is this not the behavior of nixfmt --check?

The only thing is that I wouldn't want people to add additional GitHub workflows for each part that starts using the unstable format, so instead I'd say let's not imply that the GitHub action is specific to CUDA, and instead name it something generic like check-nix-formatting.yml

Also please add an explanatory comment in this file to say that it's not stable yet with a link to NixOS/rfcs#166

@infinisil I've renamed the file and added a comment to make it clear its opt-in.

Do we want to run the check for all PRs at this point? Maybe we maintain (in the workflow definition) a list of subsystems that explicitly opt-in into enforcing the experimental style

@SomeoneSerge @piegamesde I've relaxed the run condition so it will trigger on all PRs, but have restricted it to just the relevant CUDA files.

I would make a GIthub action workflow to just run the changed files in the PR through the formatter and if any diff arises after the run -> report to the PR and block it from merging (basically happens directly because this workflow would be unsuccessful).

@gabyx @samuela yes, that is what the --check flag is for. Though, it won't print the diff itself.

Am I seeing correctly that the reformat is causing a rebuild? If so, would you please investigate this and open a bug report? Nixfmt should never change the code's semantics, and thus never cause any rebuilds.

@piegamesde @infinisil I've often wondered about those counts, especially for unfree redistributables. If they're calculated naively (e.g., does Hydra have a copy of the package cached?) then it would make sense there are always "rebuilds"; Hydra doesn't build unfree packages (like anything involving CUDA). That the Darwin count is non-zero makes me chuckle, because that's definitely one platform NVIDIA doesn't support, and one which should never be impacted by our changes (unless we, you know, break evaluation).

Is there an easy way for me to check exactly what would need to be rebuilt, or to further investigate this?

I see the current PR as the minimal amount of work necessary to format the portion of the code the @NixOS/cuda-maintainers team works on, and to keep it formatted.

I think it would be a good idea to open an issue on the nixfmt repo to create a list of requirements for a GitHub Action which runs nixfmt on a repository. I think that's a good way to gain further adoption; I know if there were a GitHub action specifically for nixfmt that I'd use it -- especially if it provided additional functionality like producing a diff of the changed files, like @gabyx suggested.

What are y'all's thoughts on something like that? I think the changes I've made to the PR (finding paths from environment variables) make it flexible enough for a small group of maintainers to opt-in prior to tree-wide formatting, but I wouldn't want to extend it further.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-04-02/42658/1

Woohoo!! thanks everyone -- excited to finally get some auto-formatting CI in nixpkgs!

Thank you all for being the first-out-of-the-gate team to opt in to RFC 166!